Refactor file upload and toast notifications #472
Conversation
Replaces react-hot-toast with a custom toast system using @radix-ui/react-toast, updating all usages and adding new UI components for toast and dialog. Refactors file upload to use a two-step process: first generating an S3 upload URL, then adding the file to the database, and adds file deletion support with confirmation dialog and S3 cleanup. Updates Prisma schema, removes unused fields, and cleans up navigation and admin settings page.
Added a check to restrict users to 2 file uploads in the demo, with a new helper function and error handling. Updated navigation items, improved landing page content, and removed unused dependencies (react-hot-toast). Added @radix-ui/react-toast, updated testimonials, and made minor content and code improvements.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the toast notification system and file upload functionality to improve user experience and maintainability. It replaces react-hot-toast with a custom shadcn-based toast system and restructures file uploads into a two-step process for better error handling.
- Replaces react-hot-toast with custom shadcn toast components
- Refactors file upload to use a two-step process (generate URL, then add to database)
- Adds file deletion functionality with confirmation dialog and S3 cleanup
- Removes "Pricing" navigation item from authenticated user interface
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| template/app/src/hooks/use-toast.ts | New custom toast hook implementation replacing react-hot-toast |
| template/app/src/components/ui/toast.tsx | New shadcn toast UI components |
| template/app/src/components/ui/toaster.tsx | Toast provider component |
| template/app/src/components/ui/dialog.tsx | New dialog components for confirmations |
| template/app/src/file-upload/operations.ts | Refactored to split file creation into URL generation and database operations |
| template/app/src/file-upload/s3Utils.ts | Added S3 file deletion utility |
| template/app/src/file-upload/FileUploadPage.tsx | Updated to use new two-step upload process and file deletion |
| template/app/src/file-upload/fileUploading.ts | Modified to accept pre-generated S3 credentials |
| template/app/src/demo-ai-app/DemoAppPage.tsx | Updated error handling to use new toast system |
| template/app/src/client/App.tsx | Added Toaster component to app root |
| template/app/src/client/components/NavBar/constants.ts | Removed Pricing from demo navigation |
| template/e2e-tests/tests/utils.ts | Fixed test navigation to use direct routing |
| throw new HttpError(401); | ||
| } | ||
|
|
||
| await context.entities.File.findUniqueOrThrow({ |
There was a problem hiding this comment.
The file ownership check queries the database but doesn't use the result. Consider combining this with the delete operation or using a more efficient approach to verify ownership before deletion.
| await context.entities.File.findUniqueOrThrow({ | |
| const deletedFile = await context.entities.File.delete({ |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
this is a decent suggestion. I will use this but change the order of deletion so it deletes the file from S3 only if the DB deletion is successful.
Refactors file deletion to delete the database record before attempting S3 deletion, ensuring the file is removed from the app even if S3 deletion fails. Adds error logging for failed S3 deletions to aid in manual cleanup. Also simplifies error handling in the file upload page and removes unused imports in the demo app page.
Added logic to decrement user credits or throw an error if out of credits in the AI demo app. Updated file upload operations to validate file existence in S3 before adding to the database, and implemented S3 file existence check utility. Minor UI and code improvements included.
|
@FranjoMindek made a couple changes based on your comments and replied to others :) |
Updated toast action hover style and icon spacing for better UI consistency. Enhanced error handling in file deletion to display specific error messages. Refined credit/subscription error message in GPT response operation for clarity and removed redundant credit decrement logic.
Replaces error state management with toast notifications for file upload errors and success. Refactors file type validation to use a new ALLOWED_FILE_TYPES_CONST and type AllowedFileTypes. Updates validation logic to throw errors instead of returning error objects, and simplifies type handling across file upload modules.
Replaces the 'key' field with 's3Key' for file storage references throughout the codebase and database schema. Updates all related logic, types, and API contracts to use 's3Key'. Adds a scheduled job to delete old files from S3 and the database. Cleans up file type validation constants and improves consistency in file upload and download operations.
|
@FranjoMindek pushed some more requested fixes along with a couple cleanup items. |
|
One meta comment: it would be better if this was split into two PRs: one for toast and the other for upload logic. I'll review the logic now - but I see Franjo did a few iterations himself, nice. |
| + const filesToDelete = await context.entities.File.findMany({ | ||
| + where: { | ||
| + createdAt: { | ||
| + lt: new Date(Date.now() - 1000 * 60 * 60 * 24 * 7), |
There was a problem hiding this comment.
I'd suggest we do
const last7Days = new Date(Date.now() - 1000 * 60 * 60 * 24 * 7);So it's more clear what is the limit. Or even better:
const dayInMiliseconds = 1000 * 60 * 60 * 24;
const timeLimitToDeleteFiles = Date.now() - 7 * dayInMiliseconds;Something along those lines.
| @@ -0,0 +1,65 @@ | |||
| import type { CleanUpOrphanedFilesS3Job } from 'wasp/server/jobs'; | |||
There was a problem hiding this comment.
This is pretty advanced stuff but it might be problematic if the user has e.g. 10000 files in S3 and we fetch 10000 IDs and then we fetch all the files from the DB and compare. It's probably inefficient, that's what I'm trying to say.
If we don't do it, there might be some orphaned files on S3 - I guess that's not that dramatic, but I like that we are trying to solve it.
I'm on the fence if we should do this or just go for the simplest kind of solution of doing nothing.
There was a problem hiding this comment.
What I'm saying is that we can't offer the user a 100% bullet proof, optimized solution out of the box. But it's better to give them something they can optimize if they need to?
| @@ -0,0 +1,65 @@ | |||
| import type { CleanUpOrphanedFilesS3Job } from 'wasp/server/jobs'; | |||
There was a problem hiding this comment.
What I'm saying is that we can't offer the user a 100% bullet proof, optimized solution out of the box. But it's better to give them something they can optimize if they need to?
removed but added suggestion to docs.
|
@infomiho and I talked about the cleanup job and decided it would be best to remove it and add a note about it in the docs in case users want to implement it themselves. |
Description
Fixes #438
Replaces react-hot-toast with a custom toast system using shadcn's toast. Also adds dialog. Refactors file upload to use a two-step process: first generating an S3 upload URL, then adding the file to the database, and adds file deletion support with confirmation dialog and S3 cleanup.
This was inspired by the
Pricingnav item that was causing confusion. In the template, I think it makes sense to show it on the LP, but not once weve logged into the Demo App, since it is available from within theAccount Settingsuser dropdown, or from the Ai scheduler toast if they run out of credits.fileup.mp4
Contributor Checklist